chore: adding psp-users CEL policy by JaydipGabani · Pull Request #537 · open-policy-agent/gatekeeper-library

JaydipGabani · 2024-05-30T00:28:28Z

What this PR does / why we need it:

Which issue(s) does this PR fix (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #541

Special notes for your reviewer:

Rego policy nehavior is -

filed in below list is reference to securityContext fields - [ runAsUser, runAsGroup, fsGroup, supplementalGroup ]

Input:

field:
      rule: MustRunAs
      ranges:
        - min: 100
          max: 200

Behavior: if field is missing from object then throw missing violation, else throw violation is field is not in required range

Input:

runAsUser:
      rule: MustRunAsNonRoot

Behavior: if runAsUser and runAsNonRoot both are missing from object then throw missing violation, else throw violation is runAsUser == 0

Input:

field:
      rule: mayRunAs
      ranges:
        - min: 100
          max: 200

Behavior: No missing field violation, but is field is present and violating the range then throw the violation

Input:

field:
  rule: RunAsAny

Behavior: No violations

maxsmythe · 2024-07-03T22:48:31Z

+              (variables.containers + variables.initContainers + variables.ephemeralContainers).filter(container,
+                container.image in variables.exemptImageExplicit ||
+                variables.exemptImagePrefixes.exists(exemption, string(container.image).startsWith(exemption)))
+          - name: badContainers


these are not bad containers yet, just containers subject to validation... candidateContainers?

maxsmythe · 2024-07-03T23:00:08Z

+          - name: missingRequiredRunAsNonRootContainers
+            expression: |
+              variables.badContainers.filter(container, 
+                has(variables.params.runAsUser) && has(variables.params.runAsUser.rule) && (variables.params.runAsUser.rule == "MustRunAsNonRoot") && ((has(container.securityContext) && !(has(container.securityContext.runAsUser) || has(container.securityContext.runAsNonRoot))) ?


We need to check that runAsNonRoot is both defined and equal to true (this is implied in the Rego check, since not will invert both "missing" and "false" to true.

Also, has(container.securityContext.runAsUser) || has(container.securityContext.runAsNonRoot) should be has(container.securityContext.runAsUser) && has(container.securityContext.runAsNonRoot) , since both conditions need to be satisfied.

maxsmythe · 2024-07-03T23:10:08Z

+          - name: invalidRunAsUserContainers
+            expression: |
+              variables.badContainers.filter(container, 
+                !(container.name in variables.processedRunAsUserContainers) &&


we should not assume that container names are globally unique, since tools like gator may not perform that kind of validation.

maxsmythe · 2024-07-03T23:14:54Z

+                    (
+                      variables.params.runAsUser.rule == "MustRunAs" ?
+                      (
+                        has(container.securityContext) && has(container.securityContext.runAsUser) ? !variables.params.runAsUser.ranges.all(range, container.securityContext.runAsUser >= range.min && container.securityContext.runAsUser <= range.max) :


I think this should be exists() instead of all(), since the constraint is satisfied as long as the user ID lies within at least one of the specified ranges.

maxsmythe · 2024-07-03T23:16:45Z

+                      (variables.params.runAsGroup.rule == "MustRunAs" || variables.params.runAsGroup.rule == "MayRunAs") && 
+                      (
+                        has(container.securityContext) && has(container.securityContext.runAsGroup) ? 
+                          !variables.params.runAsGroup.ranges.all(range, container.securityContext.runAsGroup >= range.min && container.securityContext.runAsGroup <= range.max) : 


Same comment about all() vs. exists()

maxsmythe · 2024-07-03T23:22:08Z

+          - name: invalidRunAsFsGroupContainers
+            expression: |
+              variables.badContainers.filter(container, 
+                !(variables.missingRequiredFsGroupContainers.exists(c, c.name == container.name)) &&


as above, let's not rely on names being globally unique

Processing each container type separately.

maxsmythe · 2024-07-03T23:22:47Z

+                      (variables.params.fsGroup.rule == "MustRunAs" || variables.params.fsGroup.rule == "MayRunAs") && 
+                      (
+                        has(container.securityContext) && has(container.securityContext.fsGroup) ? 
+                          !variables.params.fsGroup.ranges.all(range, container.securityContext.fsGroup >= range.min && container.securityContext.fsGroup <= range.max) : 


Same comment about all() vs. exists()

maxsmythe · 2024-07-03T23:22:53Z

+                      (
+                        has(container.securityContext) && has(container.securityContext.fsGroup) ? 
+                          !variables.params.fsGroup.ranges.all(range, container.securityContext.fsGroup >= range.min && container.securityContext.fsGroup <= range.max) : 
+                          variables.podRunAsFsGroup == null || !variables.params.fsGroup.ranges.all(range, variables.podRunAsFsGroup >= range.min && variables.podRunAsFsGroup <= range.max)


Same comment about all() vs. exists()

maxsmythe · 2024-07-03T23:23:56Z

+                      (variables.params.supplementalGroups.rule == "MustRunAs" || variables.params.supplementalGroups.rule == "MayRunAs") && 
+                      (
+                        has(container.securityContext) && has(container.securityContext.supplementalGroups) ? 
+                          !variables.params.supplementalGroups.ranges.all(range, container.securityContext.supplementalGroups.all(gp, gp>= range.min && gp <= range.max)) : 


Same comment about all() vs. exists()

maxsmythe · 2024-07-03T23:24:03Z

+                      (
+                        has(container.securityContext) && has(container.securityContext.supplementalGroups) ? 
+                          !variables.params.supplementalGroups.ranges.all(range, container.securityContext.supplementalGroups.all(gp, gp>= range.min && gp <= range.max)) : 
+                          variables.podRunAsSupplementalGroups == null || !variables.params.supplementalGroups.ranges.all(range, variables.podRunAsSupplementalGroups.all(gp, gp >= range.min && gp <= range.max))


Same comment about all() vs. exists()

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

JaydipGabani · 2024-09-12T16:18:56Z

@maxsmythe @ritazh @sozercan PTAL

maxsmythe · 2024-09-13T00:54:20Z

+            expression: |
+              !has(variables.params.exemptImages) ? [] : 
+                variables.params.exemptImages.filter(image, !image.endsWith("*"))
+          - name: exemptImages


I think we need map(container, container.image) ?

this still needs fixing

maxsmythe · 2024-09-13T01:01:29Z

+            expression: |
+              variables.containers.filter(container, 
+                !(container.image in variables.exemptImages) &&
+                has(variables.params.runAsUser) && has(variables.params.runAsUser.rule) && (variables.params.runAsUser.rule == "MustRunAsNonRoot") ?


I don't think you need a trinary operator (?) here

maxsmythe · 2024-09-13T01:05:27Z

+                (
+                  (!has(container.securityContext) || (
+                    (!has(container.securityContext.runAsNonRoot) || !container.securityContext.runAsNonRoot) && (!has(container.securityContext.runAsUser) || container.securityContext.runAsUser == 0) 
+                  )) || variables.missingRunAsNonRootGlobal


This reads like it will be true if either there is no global definition for running as non root OR if there is no local definition for running as non root.

This is incorrect. If local is defined but global is not, then there is no violation, as local definition supersedes.

Updated the code to correct this.

maxsmythe · 2024-09-13T01:13:04Z

+                (
+                  variables.params.runAsUser.rule == "RunAsAny" ? false :
+                  (
+                    variables.params.runAsUser.rule == "MustRunAsNonRoot" ?


I think the code for this will be much simpler if we:

have a variable for each rule type (MustRunAs, MayRunAs, etc.) that returns the containers violating the rule.

if the rule type is not used, this variable will return an empty list.

This will allow you to:

remove all code duplication (can go back to concatenating all containers again, since you are no longer required to filter based off container name)

Reduce the nesting/branching for each variable.

Because this will significantly change/shorten the code, I'll stop reviewing here.

Thanks for the suggestion. Made an update to CEL code, it should now be much more compact. PTAL.

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

maxsmythe · 2024-09-28T00:01:00Z

+              variables.anyObject.kind == "Pod" && has(variables.anyObject.spec.securityContext) && has(variables.anyObject.spec.securityContext.supplementalGroups) ? variables.anyObject.spec.securityContext.supplementalGroups : null
+          - name: podRunAsGroup
+            expression: |
+              variables.anyObject.kind == "Pod" && has(variables.anyObject.spec.securityContext) && has(variables.anyObject.spec.securityContext.runAsGroup) ? variables.anyObject.spec.securityContext.runAsGroup : null


Can we just put the test for kind == Pod into a matchCondition?

I don't think this constraint makes sense for anything other than Pods... we can add the same test to the Rego if necessary.

wdym by "put test for kind == Pod into a matchCondition?

I removed the condition kind == Pod from CEL and rego.

matchCondition is a field in VAP:

https://kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy/#matching-requests-matchconditions

Updated CEL to add matchCondition.

maxsmythe · 2024-09-28T00:07:54Z

+              !has(variables.anyObject.securityContext) || ((!has(variables.anyObject.securityContext.runAsNonRoot) || !variables.anyObject.securityContext.runAsNonRoot) && (!has(variables.anyObject.securityContext.runAsUser) || variables.anyObject.securityContext.runAsUser == 0))
+          - name: violatingMustOrMayRunAsUser
+            expression: |
+              has(variables.params.runAsUser) && has(variables.params.runAsUser.rule) && variables.params.runAsUser.rule == "MustRunAs" ? variables.nonExemptContainers.filter(container, (!has(container.securityContext) || !has(container.securityContext.runAsUser)) && variables.podRunAsUser == null).map(container, "Container " + container.name + " is attempting to run without a required securityContext/runAsUser") + variables.nonExemptContainers.filter(container, (has(container.securityContext) && has(container.securityContext.runAsUser) ? !variables.params.runAsUser.ranges.exists(range, container.securityContext.runAsUser >= range.min && container.securityContext.runAsUser <= range.max) : variables.podRunAsUser != null && !variables.params.runAsUser.ranges.exists(range, variables.podRunAsUser >= range.min && variables.podRunAsUser <= range.max))).map(container, "Container " + container.name + " is attempting to run as disallowed user. Allowed runAsUser: {ranges: [" + variables.params.runAsUser.ranges.map(range, "{max: " + string(range.max) + ", min: " + string(range.min) + "}").join(", ") + ", rule: " + variables.params.runAsUser.rule + "}") : []


Can we use some string/block formatting to make the various declarations more legible? It's hard to parse the assertions without formatting. This is a general comment for all of these larger code blocks.

TBH I can't review without formatting improvements... the statements bleed together.

Appologies for putting the large code block in one line. Formatted all code large code blocks. Should be easier to parse and read.

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

JaydipGabani · 2024-10-30T00:15:05Z

@maxsmythe PTAL.

Copilot

Copilot reviewed 6 out of 18 changed files in this pull request and generated no suggestions.

Files not reviewed (12)

artifacthub/library/pod-security-policy/users/1.1.0/samples/psp-pods-allowed-user-ranges/constraint.yaml: Language not supported
artifacthub/library/pod-security-policy/users/1.1.0/samples/psp-pods-allowed-user-ranges/example_allowed_exempt_image.yaml: Language not supported
artifacthub/library/pod-security-policy/users/1.1.0/samples/psp-pods-allowed-user-ranges/example_disallowed.yaml: Language not supported
artifacthub/library/pod-security-policy/users/1.1.0/suite.yaml: Language not supported
src/pod-security-policy/users/constraint.tmpl: Language not supported
src/pod-security-policy/users/src.cel: Language not supported
library/pod-security-policy/users/suite.yaml: Evaluated as low risk
library/pod-security-policy/users/samples/psp-pods-allowed-user-ranges/constraint.yaml: Evaluated as low risk
library/pod-security-policy/users/samples/psp-pods-allowed-user-ranges/example_disallowed.yaml: Evaluated as low risk
artifacthub/library/pod-security-policy/users/1.1.0/kustomization.yaml: Evaluated as low risk
artifacthub/library/pod-security-policy/users/1.1.0/samples/psp-pods-allowed-user-ranges/example_allowed.yaml: Evaluated as low risk
library/pod-security-policy/users/samples/psp-pods-allowed-user-ranges/example_allowed_exempt_image.yaml: Evaluated as low risk

maxsmythe · 2025-02-13T01:32:54Z

+                variables.nonExemptContainers.filter(container, 
+                  (!has(container.securityContext) || !has(container.securityContext.runAsGroup)) && variables.podRunAsGroup == null
+                ).map(container, 
+                  "Container " + container.name + " is attempting to run without a required securityContext/runAsGroup. Allowed runAsGroup: {ranges: [" + 


Isn't this fine for MayRunAs?

IIRC undefined is fine for MayRunAs

stale · 2025-04-15T19:36:11Z